Skip to content

fix(sync): offload tool-result images at ingest - #1729

Merged
mariusvniekerk merged 16 commits into
kenn-io:mainfrom
rodboev:pr/1718-3-offload-on-ingest
Sep 14, 2026
Merged

mariusvniekerk merged 16 commits into
kenn-io:mainfrom
rodboev:pr/1718-3-offload-on-ingest

Conversation

@rodboev

@rodboev rodboev commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

When you set tool_result_images = "offload", AgentsView moves supported tool-result images into its asset store during ingestion and keeps readable references in the archive. New sessions retain image access without storing image bytes inside tool-result text.

The setting applies to new and resynced sessions, incremental updates, and late tool results. AgentsView publishes each asset before saving its reference, and keeps the inline image if publication fails. The Settings control accepts offload, and existing inline images can be moved with agentsview db migrate --images.

Refs #1718 (slice 3b)

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (678908f)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/db/artifact_import_session.go:75-78: ApplyArtifactImportedSession sanitizes and stores imported messages without calling the policy projection used by normal batch paths. With tool_result_images set to offload, inline input_image blocks from imported artifact segments remain in SQLite and no asset is created, making storage behavior depend on the ingestion route. Apply projectSessionBatchMessages or an equivalent policy projection before sessionAndMessagesForStorage, and add an artifact-import test asserting that offload produces an asset reference and asset file.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 8s | Total: 22m52s

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (7f9dac8)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 18m22s

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (aa95b56)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/sync/engine.go:17016; internal/sync/parsediff.go:731: ParseDiff now invokes the offload projection, which writes asset files via assets.Put even though ParseDiff is report-only; re-parsing can mutate the asset store and leave unreferenced objects. Use a side-effect-free projection for ParseDiff or disable offload while comparing, and verify that the assets directory remains unchanged.

    Reported by: codex

  • internal/db/tool_result_images.go:393-406: Drop mode only strips input_image blocks. Existing agentsview_image placeholders created by offload retain their image_ref and linked markdown, so resync or imported sessions previously offloaded can continue rendering the asset after switching to drop. Extend drop projection to recognize offload placeholders, remove image_ref, and rewrite linked markdown to the normal non-asset placeholder.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 9s | Total: 33m48s

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (0bfbce0)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/db/tool_result_images.go:369; frontend/src/lib/utils/markdown.ts:815: The new asset:// placeholders resolve to relative /api/v1/assets/... URLs, but image requests cannot include the bearer Authorization header and the resolver ignores configured remote server URLs, causing 401 errors or requests to the wrong host. Load assets through an authenticated fetch/blob URL or provide a narrowly scoped authenticated asset URL mechanism, resolve asset URLs against the configured remote API base, and add an authenticated remote-rendering test.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 7s | Total: 13m42s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (0cac7b7)

Verdict: Changes require fixes for 1 finding.

High

  • frontend/src/lib/utils/markdown.ts:843; internal/server/server.go:906; desktop/src-tauri/tauri.conf.json:25: loadAssetImages replaces fetched assets with blob: URLs, but the server and Tauri CSP img-src directives omit blob:, causing browsers to block the images. Add blob: to the relevant img-src directives in the server and desktop CSP configurations, and update CSP coverage accordingly.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m46s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (ad9695c)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/sync/engine.go:17029: Image projection writes asset files during session preparation before cwd filtering, preservation vetoes, and validation, so skipped or unstored sessions can leave unreferenced assets outside the configured archive scope. Keep preparation side-effect-free and publish assets only after all session acceptance gates pass, or use a no-write projection until the final database write boundary.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 9s | Total: 20m23s

@wesm

wesm commented Sep 12, 2026

Copy link
Copy Markdown
Member

Kind of makes me think that this should be the default for new archives

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (9e37ec2)

Verdict: Changes require fixes for 3 findings.

Medium

  • internal/db/artifact_import_session.go:76: Artifact imports preserve asset:// references but artifact transport does not include the corresponding asset bytes, so imported sessions on another archive render broken images unless the asset directory is copied separately. Include referenced asset objects in artifact transport, or downgrade unresolved references to readable placeholders during import and document the behavior.

    Reported by: codex

  • internal/sync/secret_scan.go:163: Signal and secret computation uses a no-write offload projection, while storage falls back to inline content when an asset write fails. Derived values and secret match offsets can therefore refer to different content than the stored transcript. Compute derived state from the actual storage projection, including its write-failure fallback, or defer computation until the committed content is known.

    Reported by: codex

  • internal/sync/codex_staging.go:553: Large Codex transcript staging writes image bytes to the persistent asset directory before CWD filtering, validation, and publication acceptance, leaving assets behind for sessions that are excluded, rejected, or fail later publication. Keep staged parsing side-effect free by retaining inline content in the private staging store, then call assets.Put only after acceptance. Alternatively, reliably remove every asset created by a staged parse that is not committed.

    Reported by: codex (security)


Reviewers: 2 done | Synthesis: codex, 10s | Total: 18m21s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (76558a2)

Verdict: Changes require fixes for 1 finding.

High

  • frontend/src/lib/utils/markdown.ts:824-825: getAssetURL URL-encodes the entire asset:// reference, producing an invalid asset request path that the asset route cannot resolve. Strip the asset:// prefix before encoding and append only the filename.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 6s | Total: 10m50s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (5e1f3b6)

Verdict: No findings at or above medium severity.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 15m40s

mariusvniekerk pushed a commit that referenced this pull request Sep 12, 2026
Formatted tool output renders retained Codex PNG results between surrounding text and keeps large payloads within the output pane. Image-only results render as images, and Raw keeps retained inline results available for inspection.

The frontend projects retained input_image data URIs for PNG, JPEG, WebP, and GIF through the existing Markdown renderer and asset resolver. Migrated asset references keep their current behavior. Unsupported media, malformed blocks, and remote URLs remain stored text. The change stays in ToolBlock and its display helper, with no backend, storage, configuration, or locale changes.

PR #1729 covers ingest-time offload. This change covers retained inline images in Keep mode.

The reproduction came from #1735. It measured 10,812,829 pixels of horizontal scroll in a 1,623 pixel box, with zero image elements. Focused tests and Chromium proof cover the change and browser smoke tests confirm the fix.

Closes #1735


Co-authored-by: Rod Boev <rodboev@users.noreply.github.com>
@mariusvniekerk mariusvniekerk self-assigned this Sep 12, 2026
@mariusvniekerk
mariusvniekerk force-pushed the pr/1718-3-offload-on-ingest branch from 5e1f3b6 to 51dd038 Compare September 12, 2026 17:21
@roborev-ci

This comment has been minimized.

Check authenticated asset fetches and decoded blob images in the browser
instead of expecting direct asset URLs. Wait for the worker to record a
heartbeat failure before returning the independent parser error, so the
race-detector test consistently exercises both failures together.
@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (052c133)

Verdict: No findings at or above medium severity.


Reviewers: 2 done | Synthesis: codex, 12s | Total: 20m28s

@rodboev
rodboev requested a review from wesm September 13, 2026 05:09
@mariusvniekerk
mariusvniekerk merged commit 91942b3 into kenn-io:main Sep 14, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants